Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[processor/routing] Fix statement not eval in order #34999

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Frapschen
Copy link
Contributor

Description:

Rise a PR to change routes from type map[string]routingItem[E, K] to []routingItem[E, K] to fix statement not eval in order.

Link to tracking Issue: #34860

Testing:

Add three order relate tests to verify this change is work.

Documentation:

@Frapschen Frapschen requested a review from a team September 4, 2024 09:29
@github-actions github-actions bot added the processor/routing Routing processor label Sep 4, 2024
@Frapschen Frapschen force-pushed the fix-router-random-statements branch from 60b08a3 to 471a0e4 Compare September 4, 2024 15:47
@github-actions github-actions bot added the cmd/otelcontribcol otelcontribcol command label Sep 5, 2024
@@ -671,7 +671,7 @@ require (
github.com/open-telemetry/otel-arrow v0.25.0 // indirect
github.com/opencontainers/go-digest v1.0.0 // indirect
github.com/opencontainers/image-spec v1.1.0 // indirect
github.com/opencontainers/runc v1.1.14 // indirect
github.com/opencontainers/runc v1.1.13 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this change intended?

FromAttribute: "__otel_enabled__",
AttributeSource: resourceAttributeSource,
DefaultExporters: []string{"otlp"},
Table: []RoutingTableItem{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I find this test a bit confusing: the first two conditions will never be true for different reasons and the third is only resulting in true if the deletion happens based on the value of the key used in the where clause.

The statements could be just simple route statements?

route() where resource.attributes["__otel_enabled__"] == nil
route() where resource.attributes["__otel_enabled__"] == "true"
route() where resource.attributes["__otel_enabled__"] == "false" // this is the only one resulting in true

Or even better:

route() where resource.attributes["non-matching"] != nil // non-matching is not set, this is never true
route() where resource.attributes["non-matching"] == "true" // non-matching is not set, therefore, not "true"
route() where resource.attributes["matching"] == "true" // matches!

And in the main test code, have this attribute instead:

rl.Resource().Attributes().PutStr("matching", "true")

Copy link
Contributor Author

@Frapschen Frapschen Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpkrohling The test is designed to run 5 times, and I expected it to produce the same result each time. This is how expected statements work. If the statements are executed randomly (map[string]routingItem[E, K]), they can't yield consistent results.

BWT, the statements is from the issue's description. It can demonstrate that statements are executed in order or randomly will affect the routing results. so I used it in the test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running them 5 times is fine, my concern is about the readability of the test. While the statements on the issue description were enough to reproduce the problem, the tests should be easily understandable to our future selves without having to come back to this issue to get the full picture, IMO.

e, ok := r.routes[key]
if !ok {
return r.defaultExporters
for _, route := range r.routes {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what are the performance implications of this change. This code is in the hot path. If you are using this change in production already, are you able to provide some insights here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested it in production yet, but according to the changed code, it does not break the loop behavior. Whether routes is a map or a slice, it always loops through all the elements in the routes. So, I would say it will not affect the performance negatively.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not concerned about the behavior, I'm concerned about the performance. I just double-checked, and this doesn't seem like it's in the hot path, called only on the Start path. In any case, it would make me more comfortable if we had a benchmark comparing how it was before and how it is now.

Copy link
Contributor

github-actions bot commented Oct 3, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 3, 2024
@Frapschen Frapschen removed the Stale label Oct 8, 2024
@Frapschen Frapschen force-pushed the fix-router-random-statements branch from 1f87da8 to c600907 Compare October 8, 2024 08:28
@Frapschen Frapschen requested a review from a team as a code owner October 8, 2024 08:28
@Frapschen
Copy link
Contributor Author

sorry for the delay @jpkrohling PTAL

Signed-off-by: Murphy Chen <[email protected]>
Signed-off-by: Murphy Chen <[email protected]>
Signed-off-by: Murphy Chen <[email protected]>
Signed-off-by: Murphy Chen <[email protected]>
Signed-off-by: Murphy Chen <[email protected]>
@Frapschen Frapschen force-pushed the fix-router-random-statements branch from da26ea7 to 72877ae Compare October 9, 2024 01:33
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 23, 2024
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinging @jpkrohling please confirm whether your comments have been addressed

@github-actions github-actions bot removed the Stale label Nov 1, 2024
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 19, 2024
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The concerns about the readability of the tests weren't addressed yet, and tests for metrics and traces are missing.

@github-actions github-actions bot removed the Stale label Dec 3, 2024
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/otelcontribcol otelcontribcol command processor/routing Routing processor Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants